Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plain ECDSA implementation #21

Merged
merged 5 commits into from
Dec 19, 2023
Merged

Plain ECDSA implementation #21

merged 5 commits into from
Dec 19, 2023

Conversation

arcuo
Copy link
Collaborator

@arcuo arcuo commented Dec 14, 2023

Renamed functions to show that it runs bedoza.
Implemented plain ECDSA from https://cryptobook.nakov.com/digital-signatures/ecdsa-sign-verify-messages

@arcuo arcuo self-assigned this Dec 14, 2023
Copy link
Owner

@mzacho mzacho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments on the naming, but looks great! :)

@@ -23,18 +23,18 @@ use crate::{
/// 1. Sign message
/// 2. Verify signature
/// 3. PROFIT!
pub fn run_ecdsa(message: Nat) {
pub fn run_ecdsa_bedoza(message: Nat) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of this bedoza / plain naming scheme -

Can we use threshold_sign and sign instead (I've been keeping this to myself, but I also generally think including names of parameters as part of a function name, such as fn sign_message(message: str) is an anti-pattern :))? And perhaps run_threshold_ecdsa for the runner?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to spend a lot of energy arguing naming though, so just view this as a suggestion and mark the comment as resolved :)

}

#[test]
fn test_threshold_ecdsa_positive() {
fn test_threshold_ecdsa_plain_positive() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... Continuing the comment on the naming: I think of 'threshold signing' as signing using MPC and thus 'test_threshold_ecdsa_plain' doesn't make much sense IMO :)

/// Run a ECDSA protocol without BeDOZa
/// Used for benchmarking
/// Uses the protocol from https://cryptobook.nakov.com/digital-signatures/ecdsa-sign-verify-messages
pub fn run_ecdsa_plain(message: Nat) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're only using this for benchmarking, couldn't we just put it in the test module? I.e under

#[cfg(test)]
mod test_plain { ... }

@arcuo arcuo merged commit 3809c2d into main Dec 19, 2023
1 check passed
@arcuo arcuo deleted the plain-ecdsa branch December 19, 2023 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants